Refactor container chain spawning logic to not need require embeded node outside of collation #627

nanocryk · 2024-07-16T08:39:42Z

Currently the container chain spawning logic requires an embeded orchestrator client, which prevents using it in data preservers nodes that may interact with the orchestrator through a WebSocket connection. The linked PR modifies the OrchestratorChainInterface to add functions to get necessary data without the need for a client. When collating a client is still needed, as a data preserver node will not collate on the orchestrator chain.

pallets/registrar/src/lib.rs

tmpolaczyk · 2024-07-16T14:19:23Z

node/src/container_chain_spawner.rs

@@ -796,7 +810,7 @@ fn handle_update_assignment_state_change(
 /// interrupted before it finished downloading the state, in that case the node will use warp sync.
 /// If it was interrupted during the block history download, the node will use full sync but also
 /// finish the block history download in the background, even if sync mode is set to full sync.
-fn select_sync_mode(
+pub fn select_sync_mode_using_client(


In this function we only need the orchestrator_client to check an edge case for when the container chain is still at block 0. We could also use the relay_chain_interface and get the latest finalized block from there, would that be better for the ws node?

we could yes, in the end we are fetching that information from the relay itself

Which function in the relay interface should be called? Indeed that would allow to get rid of the generic and not need the orchestrator client.

Probably paras->heads, but you would need to parse the header to get the block number. There are some utility functions in pallet_author_noting, such as author_from_log. @girazoki any better ideas? Maybe there is some other storage that only exists after the parachain has included its first block?

Can we tackle that in a separate PR?

yes let's do it as a separate PR. But it would definitely make it more compatible with solo-chains

Cargo.toml

github-actions · 2024-07-17T14:10:32Z

Coverage Report

(master)

@@                              Coverage Diff                               @@
##           master   jeremy-orchestrator-spawner-without-client      +/-   ##
==============================================================================
- Coverage   67.32%                                       66.88%   -0.44%     
  Files         255                                          255              
+ Lines       44448                                        44587     +139     
==============================================================================
- Hits        29922                                        29819     -103     
+ Misses      14526                                        14768     +242

Files Changed	Coverage
/node/src/chain_spec/dancebox.rs	94.82% (+0.02%)	🔼
/node/src/chain_spec/flashbox.rs	70.87% (+0.12%)	🔼
/node/src/service.rs	19.97% (-0.88%)	🔽
/pallets/registrar/src/lib.rs	88.82% (-0.04%)	🔽
/runtime/dancebox/tests/common/mod.rs	98.63% (-0.01%)	🔽
/runtime/flashbox/tests/common/mod.rs	95.96% (-0.05%)	🔽
/solo-chains/runtime/starlight/tests/common/mod.rs	90.31% (-0.17%)	🔽

Coverage generated Mon Jul 22 08:32:43 UTC 2024

tmpolaczyk · 2024-07-17T15:07:49Z

test/scripts/downloadChainSpec.ts

@@ -49,7 +49,7 @@ yargs(hideBin(process.argv))
                }
                process.stdout.write(`Done ✅\n`);
                const onChainGenesisData = await api.createType(
-                    "TpContainerChainGenesisDataContainerChainGenesisData",
+                    "DpContainerChainGenesisDataContainerChainGenesisData",


Remember to mention this in breaking changes, this will probably break all the registration scripts we have

Anything that uses api.createType("Tp... instead of Dp will stop working?

yes but I dont think we use it

maybe the dapp does

tmpolaczyk · 2024-07-18T09:41:13Z

node/src/container_chain_spawner.rs

@@ -235,6 +246,8 @@ async fn try_spawn(
    );

    if !start_collation {
+        collation_params = None;


I forgot why we used both validator && start_collation instead of just one boolean, here is a summary:

The reason start_collation exists as a separate arg to validator is that in the old days instead of restarting the container chain node we used collate_on to sent a message to enable the collator. So we would have to pass collation_params set to Some and start_collation set to false, and then use the collate_on closure to start collation later.

Now we don't have that collate_on, so just setting to None here is ok. Although I'm considering bringing collate_on back, for parathreads, but that's an issue for the future.

mayeb we should add a comment for this then

I can do that in the next refactor

tmpolaczyk · 2024-07-18T09:42:13Z

node/src/container_chain_spawner.rs

@@ -70,11 +69,21 @@ const MAX_DB_RESTART_TIMEOUT: Duration = Duration::from_secs(60);
 /// Assuming a syncing speed of 100 blocks per second, this will take 5 minutes to sync.
 const MAX_BLOCK_DIFF_FOR_FULL_SYNC: u32 = 30_000;

+pub trait TSelectSyncMode:
+    Send + Sync + Clone + 'static + (Fn(bool, ParaId) -> sc_service::error::Result<SyncMode>)


Using Fn() as a trait bound is very ugly, promise to remove this trait as soon as possible in a future PR

Haha, as we discussed we should be able to remove SelectSyncMode entirely :)

tmpolaczyk · 2024-07-18T09:43:51Z

Zombienet tests are failing with error:

2024-07-17 15:32:33 [Orchestrator] Failed to start container chain 2002: Other: No genesis data registered for container chain id 2002

so indeed some registration script probably broke

tmpolaczyk · 2024-07-18T12:31:47Z

so indeed some registration script probably broke

Actually the registration seems to work fine, it's just the node that fails to read the genesis data for some reason

nanocryk · 2024-07-19T07:56:36Z

It was due to the fact that I was storing an orchestrator block hash in the main structure itself, which is probably the genesis block in that test. Since 2002 is registered on the fly later, the spawner was not able to get the genesis data. I modified (again 💀 ) the chain interface to allow fetching the best or finalized head (the relay interface have that too), which should fix the issue.

Currently I use best head to do like previous code, but it might be better to use the finalized one?

tmpolaczyk · 2024-07-19T08:49:17Z

Currently I use best head to do like previous code, but it might be better to use the finalized one?

In practice it shouldn't matter because the values we read only change after 1 session, but I guess using finalized is more consistent with the rx_loop function, which reacts on finalized blocks (because otherwise it would need to unspawn a container chain in case of block revert).

…-spawner-without-client

girazoki · 2024-07-22T08:53:13Z

Currently I use best head to do like previous code, but it might be better to use the finalized one?

In practice it shouldn't matter because the values we read only change after 1 session, but I guess using finalized is more consistent with the rx_loop function, which reacts on finalized blocks (because otherwise it would need to unspawn a container chain in case of block revert).

I did create an issue to change just this, but let's do it as a separate PR

pallets/registrar/src/lib.rs

nanocryk added 5 commits July 15, 2024 17:53

move in/out crates between tanssi and dancekit

b726aff

remove generic MaxLengthTokenSymbol

03f67bd

remove need for embeded node + fmt

395b7f9

update OrchestratorChainInterface

49e6425

docs

a2c7a5d

tmpolaczyk reviewed Jul 16, 2024

View reviewed changes

pallets/registrar/src/lib.rs Show resolved Hide resolved

tmpolaczyk reviewed Jul 16, 2024

View reviewed changes

girazoki reviewed Jul 16, 2024

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

nanocryk added 2 commits July 17, 2024 13:43

fix issues following dancekit bump

f12fbef

fix polkadot-sdk commit hash in lock

abb8684

nanocryk added 2 commits July 17, 2024 16:15

clippy + try to fix ts tests

a6d0ef5

fmt

45a16d8

tmpolaczyk reviewed Jul 17, 2024

View reviewed changes

nanocryk added the breaking Needs to be mentioned in breaking changes label Jul 17, 2024

tmpolaczyk reviewed Jul 18, 2024

View reviewed changes

fix not getting genesis data in best block

91527ce

nanocryk added 3 commits July 19, 2024 13:59

add back removed log

e5ff9c6

Merge remote-tracking branch 'origin/master' into jeremy-orchestrator…

e80e9c0

…-spawner-without-client

Merge remote-tracking branch 'origin/master' into jeremy-orchestrator…

01f5df8

…-spawner-without-client

girazoki reviewed Jul 22, 2024

View reviewed changes

pallets/registrar/src/lib.rs Show resolved Hide resolved

girazoki approved these changes Jul 22, 2024

View reviewed changes

tmpolaczyk approved these changes Jul 22, 2024

View reviewed changes

nanocryk merged commit bf28d34 into master Jul 23, 2024
36 checks passed

nanocryk deleted the jeremy-orchestrator-spawner-without-client branch July 23, 2024 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor container chain spawning logic to not need require embeded node outside of collation #627

Refactor container chain spawning logic to not need require embeded node outside of collation #627

nanocryk commented Jul 16, 2024

tmpolaczyk Jul 16, 2024

girazoki Jul 16, 2024

nanocryk Jul 17, 2024

tmpolaczyk Jul 17, 2024

nanocryk Jul 18, 2024

girazoki Jul 22, 2024

github-actions bot commented Jul 17, 2024 •

edited

Loading

tmpolaczyk Jul 17, 2024

girazoki Jul 22, 2024

tmpolaczyk Jul 22, 2024

girazoki Jul 22, 2024

girazoki Jul 22, 2024

tmpolaczyk Jul 18, 2024

girazoki Jul 22, 2024

tmpolaczyk Jul 22, 2024

tmpolaczyk Jul 18, 2024

nanocryk Jul 19, 2024

tmpolaczyk commented Jul 18, 2024

tmpolaczyk commented Jul 18, 2024

nanocryk commented Jul 19, 2024

tmpolaczyk commented Jul 19, 2024

girazoki commented Jul 22, 2024

Refactor container chain spawning logic to not need require embeded node outside of collation #627

Refactor container chain spawning logic to not need require embeded node outside of collation #627

Conversation

nanocryk commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmpolaczyk commented Jul 18, 2024

tmpolaczyk commented Jul 18, 2024

nanocryk commented Jul 19, 2024

tmpolaczyk commented Jul 19, 2024

girazoki commented Jul 22, 2024

github-actions bot commented Jul 17, 2024 •

edited

Loading